Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added initialisation of creationTimestamp when creating instances of NodeLatencyStats . #6574

Merged
merged 1 commit into from
Aug 8, 2024

Conversation

hkiiita
Copy link
Member

@hkiiita hkiiita commented Jul 30, 2024

Attempts to fix issue #6569 about creationTimestamp field is always null in NodeLatencyStats.
Awaiting review.

Copy link
Contributor

@antoninbas antoninbas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unit tests need to be updated.
They are probably failing at the moment. You can run them locally with go test -v ./pkg/apiserver/registry/stats/nodelatencystats/.
Unit tests should validate that when Create is invoked without a timestamp, the timestamp is added, and that when Create is invoked with a timestamp, the timestamp is not modified.

Comment on lines 54 to 56
return &statsv1alpha1.NodeLatencyStats{
ObjectMeta: metav1.ObjectMeta{CreationTimestamp: metav1.Now()},
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this change is needed or even makes sense. I would not modify New as part of this PR.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, worked on it.

@hkiiita
Copy link
Member Author

hkiiita commented Jul 31, 2024

Unit tests need to be updated. They are probably failing at the moment. You can run them locally with go test -v ./pkg/apiserver/registry/stats/nodelatencystats/. Unit tests should validate that when Create is invoked without a timestamp, the timestamp is added, and that when Create is invoked with a timestamp, the timestamp is not modified.

Added new tests and refactored existing ones.

@hkiiita
Copy link
Member Author

hkiiita commented Jul 31, 2024

@antoninbas Please review.

summary *statsv1alpha1.NodeLatencyStats
nodeName string
expectedObj runtime.Object
err error
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this field is not used, please remove

tests := []struct {
name string
summary *statsv1alpha1.NodeLatencyStats
nodeName string
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this field is not used, please remove

Comment on lines 87 to 85
} else {
assert.NotEqual(t, tt.expectedObj, obj)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this doesn't make sense IMO. Just because a timestamp is added by Create in this case doesn't mean that assert.NotEqual is the right thing to check. Especially when you have a field called tt.expectedObj, which implies that you are expecting the function to return a specific object.

usually the best way to deal with this is to add a clock.Clock (https://pkg.go.dev/k8s.io/utils/clock#Clock) to the object being tested (in this case, to the REST object). In the "normal" case, the clock is set to RealClock, but for tests you can set it to FakeClock. This way you can freeze time and predict what the timestamp will be / should be in the returned object. There are multiple examples in the Antrea code base, so you can look for them.

name string
summary *statsv1alpha1.NodeLatencyStats
nodeName string
expectedObj runtime.Object
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I would use the *statsv1alpha1.NodeLatencyStats type for this as well

nodeName string
expectedObj runtime.Object
err error
timeAdded bool
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the name is a bit confusion IMO. timeAdded to me means that the time is added by the Create function, but that's not how you use it. I would suggest something less ambiguous, such as timestampMissing.

}

func TestRESTGet(t *testing.T) {
addedTime := metav1.Now()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rename to creationTimestamp

same comment for subsequent tests

@@ -16,6 +16,7 @@ package nodelatencystats

import (
"context"
testing2 "k8s.io/utils/clock/testing"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

testing2 is not a good name for an import, use clocktesting

pkg/apiserver/registry/stats/nodelatencystats/rest_test.go Outdated Show resolved Hide resolved
@@ -28,29 +29,56 @@ import (
statsv1alpha1 "antrea.io/antrea/pkg/apis/stats/v1alpha1"
)

var (
fakeClock = testing2.NewFakeClock(time.Now())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's better to have a separate clock instance for each test IMO, as some tests may modify the clock

Comment on lines 80 to 85
if tt.name == "create with time not added" {
assert.Equal(t, metav1.Time{Time: fakeClock.Now()}, obj.(*statsv1alpha1.NodeLatencyStats).CreationTimestamp)

} else {
assert.Equal(t, tt.summary.CreationTimestamp, obj.(*statsv1alpha1.NodeLatencyStats).CreationTimestamp)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not ok. You should not modify the test logic based on the test case name, otherwise it defeats the purpose of having table-driven tests. You need to use expectedObj which you are ignoring at the moment.

ctx := context.Background()

_, err := r.Create(ctx, summary, nil, nil)
require.NoError(t, err)
obj, err := r.ConvertToTable(ctx, summary, nil)
require.NoError(t, err)
assert.Equal(t, expectedCells, obj.Rows[0].Cells)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't remove the trailing newline character

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this has not been addressed yet

Comment on lines 47 to 49
func NewREST() *REST {
func NewREST(clock clock.Clock) *REST {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that the clock is only necessary for testing, let's use the following approach instead:

func NewREST() *REST {
	return newRESTWithClock(cloc.RealClock{})
}

func newRESTWithClock(clock clock.Clock) *REST {
	return &REST{
		indexer: cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{}),
		clock:   clock,
	}
}

This way in apiserver.go you can keep calling NewREST() (no clock argument required) and in tests you can call newRESTWithClock.

@hkiiita
Copy link
Member Author

hkiiita commented Aug 5, 2024

Hi @antoninbas,

I understand and am aware that this has taken a while, i appreciate your patience as I've been refining the previous commit based on your guidance. I've worked on the changes, specifically addressing the use of fakeClock and its advancement in line with using expectedObj, as rightly pointed and suggested by you.

I encountered some confusion initially regarding the fakeClock functionality and how to detect its advancement. However, after reviewing the code base and running some local experiments, I believe I have arrived at a solution that aligns with your recommendations.

Request you to please review the updated commit.

Thanks

PeerNodeLatencyStats: nil,
},
expectedObj: &statsv1alpha1.NodeLatencyStats{
ObjectMeta: metav1.ObjectMeta{Name: "node1", CreationTimestamp: metav1.Time{Time: fakeClock.Now().Add(2 * time.Minute)}},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not good because your test case assumes that it will run second and that the clock will have been advanced twice as a result. A key principle of table-driven test cases is that test cases should not depend on each other. They should be runnable individually, independently, in any order, and sometimes even in parallel.

The test should be like this:

func TestRESTCreate(t *testing.T) {
        ctx := context.Background()
        now := time.Now()
        const timeStep = 1*time.Minute
	tests := []struct {
		name        string
		summary     *statsv1alpha1.NodeLatencyStats
		expectedObj *statsv1alpha1.NodeLatencyStats
	}{
		{
			name: "create with existing timestamp",
			summary: &statsv1alpha1.NodeLatencyStats{
				ObjectMeta:           metav1.ObjectMeta{Name: "node1", CreationTimestamp: metav1.Time{Time: now}},
				PeerNodeLatencyStats: nil,
			},
			expectedObj: &statsv1alpha1.NodeLatencyStats{
				ObjectMeta:           metav1.ObjectMeta{Name: "node1", CreationTimestamp: metav1.Time{Time: now}},
				PeerNodeLatencyStats: nil,
			},
		},
		{
			name: "create with no existing timestamp",
			summary: &statsv1alpha1.NodeLatencyStats{
				ObjectMeta:           metav1.ObjectMeta{Name: "node1"},
				PeerNodeLatencyStats: nil,
			},
			expectedObj: &statsv1alpha1.NodeLatencyStats{
				// the test case advances the clock by timeStep
				ObjectMeta:           metav1.ObjectMeta{Name: "node1", CreationTimestamp: metav1.Time{Time: now.Add(timeStep)}},
				PeerNodeLatencyStats: nil,
			},
		},
	}

	for _, tt := range tests {
		t.Run(tt.name, func(t *testing.T) {
	                 fakeClock := clocktesting.NewFakeClock(now)
			r := newRESTWithClock(fakeClock)
			fakeClock.SetTime(now.Add(timeStep))
			obj, err := r.Create(ctx, tt.summary, nil, nil)
			require.NoError(t, err)
			assert.Equal(t, tt.expectedObj, obj)
		})
	}
}

you can use the code as is. I also updated the test case names for more clarity.

ctx := context.Background()

_, err := r.Create(ctx, summary, nil, nil)
require.NoError(t, err)
obj, err := r.ConvertToTable(ctx, summary, nil)
require.NoError(t, err)
assert.Equal(t, expectedCells, obj.Rows[0].Cells)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this has not been addressed yet

@@ -210,12 +242,12 @@ func TestRESTConvertToTable(t *testing.T) {
}
expectedCells := []interface{}{"node1", 2, "1.5ms", "2ms"}

r := NewREST()
r := newRESTWithClock(fakeClock)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this test needs a fake clock, so you can remove it

@@ -105,6 +134,7 @@ func TestRESTGet(t *testing.T) {
}

func TestRESTDelete(t *testing.T) {
fakeClock := clocktesting.NewFakeClock(time.Now())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

follow the example I gave above: now := time.Now()

then in each subtest, create a separate clock with fakeClock := clocktesting.NewFakeClock(now)

}

func TestRESTGet(t *testing.T) {
fakeClock := clocktesting.NewFakeClock(time.Now())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

follow the example I gave above: now := time.Now()

then in each subtest, create a separate clock with fakeClock := clocktesting.NewFakeClock(now)

@hkiiita
Copy link
Member Author

hkiiita commented Aug 6, 2024

Hi, @antoninbas request you to please have a look.

Copy link
Contributor

@antoninbas antoninbas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a couple of nits, otherwise, LGTM

t.Run(tt.name, func(t *testing.T) {
fakeClock := clocktesting.NewFakeClock(now)
r := newRESTWithClock(fakeClock)
fakeClock.SetTime(now.Add(timeStep))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
fakeClock.SetTime(now.Add(timeStep))
fakeClock.Step(timeStep)

I may have forgotten to suggest it in my previous review

@@ -219,3 +253,4 @@ func TestRESTConvertToTable(t *testing.T) {
require.NoError(t, err)
assert.Equal(t, expectedCells, obj.Rows[0].Cells)
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove this extra line

@@ -156,20 +189,21 @@ func TestRESTDelete(t *testing.T) {
}

func TestRESTList(t *testing.T) {
fakeClock := clocktesting.NewFakeClock(time.Now())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: introduce now := time.Now() for consistency with other tests and less verbosity

@hkiiita
Copy link
Member Author

hkiiita commented Aug 7, 2024

@antoninbas Yes, did the changes, please check .

Copy link
Contributor

@antoninbas antoninbas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hkiiita the code looks good to me but you have some CI failures (for code linting). You need to run make golangci-fix locally and commit the changes.

@hkiiita
Copy link
Member Author

hkiiita commented Aug 7, 2024

@antoninbas Thanks . Yes ran the command and committed.

@antoninbas
Copy link
Contributor

/test-all

@antoninbas
Copy link
Contributor

@hkiiita the DCO check is failing - please sign your commits as per our contribution guideline
It may be a good opportunity to squash all your commits as well.

- Initialized fakeClock for each test case.
- Refactored NewRest function to prevent API breakage and updated tests.
- Improved test logic and fixed lint issues.
- Made various improvements for better code readability and efficiency.

Signed-off-by: HEMANT KUMAR <[email protected]>
@hkiiita
Copy link
Member Author

hkiiita commented Aug 8, 2024

@antoninbas Thanks for it. Squashed the old commits into a single one.

@antoninbas
Copy link
Contributor

/skip-all

@antoninbas
Copy link
Contributor

Thanks for your contribution!

@antoninbas antoninbas merged commit 6e68789 into antrea-io:main Aug 8, 2024
55 of 58 checks passed
@hkiiita
Copy link
Member Author

hkiiita commented Aug 8, 2024

Thanks for your contribution!

@antoninbas Thanks to you for being so patient and helping. Really appreciate that . On the other hand i definitely ended up learning a couple of things from this . I will continue to learn more and get better with things .

@antoninbas
Copy link
Contributor

@hkiiita if you intend to apply to the LFX mentorship program for this term's project, make sure to complete our test task: #6590

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants